solana: multi-tenant deployment#875
Conversation
001c6a6 to
73f9855
Compare
|
Claude finished @johnsaigle's task in 3m 35s —— View job Review: PR #875 — Solana Multi-Tenant Deployment
OverviewThis is a well-architected PR that transforms the Solana NTT program from a singleton-per-program model (v3) into a multi-instance/multi-tenant architecture (v4). Each program can now host multiple independent NTT deployments, where each deployment is identified by a caller-provided Key architectural changes:
The PR is ~15k lines with changes across 89 files. Here are my findings: Solana Program — Positive FindingsCross-instance isolation is thorough. Every critical PDA (outbox rate limit, inbox rate limit, peers, registered transceivers, token authority, session authority, emitter, inbox items, LUT) correctly incorporates OutboxItem binding is correct. The new Redeem properly validates the recipient manager identity against Ownership transfer simplification is clean. Removing the BPF loader CPI (~90 lines) and replacing it with a pure data mutation in Issues & Suggestions1.
|
| Aspect | v3 | v4 |
|---|---|---|
| Instances per program | 1 | Multiple |
| Instance identity | Program ID | Config keypair |
| PDA scoping | Global | Per-instance (config.key()) |
| Ownership | Tied to BPF upgrade authority | Independent |
| Manager identity (on-wire) | Program ID bytes | Config pubkey bytes |
| Ownership transfer | BPF loader CPI (~90 LOC) | Pure data mutation (~3 LOC) |
Overall this is a well-executed architectural change with strong cross-instance isolation. The issues noted above are relatively minor — the most actionable one is the set-mint-authority instance awareness gap.
0ad1b81 to
44239cf
Compare
johnsaigle
left a comment
There was a problem hiding this comment.
Two potential issues that I'd appreciate clarification on. 🙏🏻
I have a few design questions as well -- maybe they would be best recorded as GitHub issues or else in a design document somewhere.
- Have you considered whether these changes a) are compatible with Fogo or b) how this change impacts SVM <-> SVM interactions? I haven't considered this in my review because I wanted to check whether it's worth spending time on first.
| space = 8 + ValidatedTransceiverMessage::<TransceiverMessageData<NativeTokenTransfer<Payload>>>::INIT_SPACE, | ||
| seeds = [ | ||
| ValidatedTransceiverMessage::<TransceiverMessageData<NativeTokenTransfer<Payload>>>::SEED_PREFIX, | ||
| config.key().as_ref(), |
There was a problem hiding this comment.
I think we ought to add a check here such that the recipient_ntt_manager matches the instance key. Otherwise I think you can take a VAA intended for an Instance A and create the transceiver message for Instance B. That incorrectly creates the transceiver message
I believe the impact here is low-to-nil for a few reasons:
- The exist seeds include the emitter chain and ID as seeds
- Solana only allows one peer per Wormhole emitter chain
- The IDs increase monotically on the sender side
This should suffice to make the account resulting from the seeds truly unique. However if all of the above weren't true, it would be possible to e.g. relay messages sent from EVM peers intended for given SVM instances to the wrong SVM instance, and potentially front-run transceiver message accounts and DoS them via the init constraint.
Let me know if you agree with this read. IMO this is definitely worth a comment because understanding the security here relies on keeping a lot of NTT subtlety in your head.
There was a problem hiding this comment.
This should be applied to both files:
solana/programs/example-native-token-transfers/src/transceivers/wormhole/instructions/receive_message.rs
solana/programs/ntt-transceiver/src/wormhole/instructions/receive_message.rs
428e8df to
de8534b
Compare
- bind OutboxItem to manager instance - decouple BPF upgrade authority from Config owner
Multi-tenant Solana NTT (>= v4) PDAs are scoped by the per-deployment Instance pubkey, so every CLI surface that reads or writes a manager needs to know it: - ChainConfig grows an optional `instance` field; SolanaDeploymentResult threads it back from `deploy()`. - pullChainConfig / nttFromManager take an optional `solanaInstance` arg, plumbed through every caller (add-chain, upgrade, clone, transfer-ownership, set-mint-authority, solana subcommands, config-mgmt's pull loop). - deploySvm gains a multi-tenant branch: generates (or loads via --instance-key) an Instance keypair, sets contracts.ntt.instance before SDK construction, co-signs `initialize`, and returns the instance pubkey alongside the program address. addSolanaInstance no longer rethrows on initializeOrUpdateLUT failure (matches deploySvm's swallow-on-LUT shape; lets dev environments without the wormhole core bridge still write deployment.json). - `ntt solana token-authority --instance <pubkey>` derives the per-instance PDA before mint-authority handoff. SolanaNtt's constructor now refuses the v4-without-instance and v3-with-instance footguns at construction time (the PDA factory accepts an optional config arg for back-compat, so without this the SDK silently falls back to legacy singleton derivations against a multi-tenant manager). Old "isV4" branches renamed to "multiTenant" in cli/src/solana/deploy.ts to capture the property we're checking. Local cli/test/solana.sh: - export COPYFILE_DISABLE=1 (macOS' AppleDouble metadata files break solana-test-validator's genesis-archive unpacker). - airdrop with --commitment finalized — `solana program deploy` uses --commitment finalized and was racing finalization on a fresh airdrop, surfacing as a bogus "insufficient funds" against a 50-SOL account. - new v4 multi-tenant section: asserts `ntt upgrade` is blocked at the v3->v4 boundary by canUpgrade(), then patches Anchor.toml + lib.rs to declare a locally-keypair'd id, rebuilds, deploys, and walks `add-chain --instance-of` end-to-end. cleanup() trap unconditionally restores the patched source files on exit.
cli/e2e/e2e-solana.test.ts spins up its own `solana-test-validator` loaded with the wormhole core bridge + post-message shim + verify-vaa shim as genesis programs (mirroring solana/Anchor.toml's [[test.genesis]] setup) plus the local v4 NTT .so at its declared id, then drives `ntt` end-to-end via Bun.spawn. Three tests: - `ntt init Mainnet` writes deployment.json with the expected shape. - `add-chain --instance-of` creates a multi-tenant Instance under the pre-loaded program (skipping deploy) and persists the `instance` pubkey alongside `manager` in deployment.json. - `ntt upgrade Solana --ver 4.0.0` from a synthetic v3 deployment.json is blocked by canUpgrade() with the migration-steer error message. Logging knobs: NTT_E2E_DEBUG=1 one progress line per `ntt` invocation NTT_E2E_VERBOSE=1 full stdout+stderr per invocation On failure, full stdout+stderr is dumped through the thrown error. The validator's stdout/stderr is unconditionally appended to /tmp/ntt-e2e-validator.log for `tail -f`-style real-time inspection. Per-test timeouts are set in-file so `bun test cli/e2e/e2e-solana.test.ts` runs without a `--timeout` flag: validator boot ~10s, full add-chain ~70s, upgrade-block <1s. cli/src/index.ts: `.parseAsync().then(() => process.exit(0))` instead of `.parse()`. Without the explicit exit, the Solana SDK's `Connection` leaves a websocket subscription open after a successful command and the CLI hangs indefinitely waiting for an event-loop drain that won't come. This bites real users too — `ntt add-chain`/`upgrade`/`push` exiting cleanly is what everyone expects. .github/workflows/cli.yml: adds `test-cli-solana-e2e` job mirroring solana.yml's `anchor-test` setup (bun 1.3.4, solana 1.18.26, anchor 0.29.0) plus `make sdk` to produce the v4 .so, then runs the bun suite. Uploads /tmp/ntt-e2e-validator.log as an artifact on failure so CI-only flakes are debuggable.
The CLI imports several @wormhole-foundation/sdk-*-ntt workspace
packages that resolve via bun's workspace symlinks under root
node_modules. Without 'bun ci' at the repo root, those symlinks
don't exist and 'ntt' fails on first import:
Cannot find module '@wormhole-foundation/sdk-evm-ntt'
from cli/src/index.ts
Mirrors the pattern in test-cli-unit, which already does 'bun ci'
first.
`bun run --cwd cli test:e2e` was a glob over `e2e/*.test.ts`, which
meant the EVM job in cli.yml started running the new Solana e2e
suite too. The EVM CI runner has no `solana-test-validator`, so
the suite errored out in beforeAll and the job failed.
Split into:
test:e2e:evm — anvil-only (used by the EVM CI job)
test:e2e:solana — solana-test-validator-only (used locally;
test-cli-solana-e2e runs the file directly)
test:e2e — both, sequentially (for local 'run everything')
`make sdk` calls `bun run build:solana` which only builds sdk-definitions-ntt + sdk-solana-ntt. The CLI also imports @wormhole-foundation/sdk-evm-ntt and sdk-sui-ntt — without their dist/ populated, bun resolves the workspace symlink to a package whose main/module fields point at non-existent files and the resolver surfaces it as 'Cannot find module sdk-evm-ntt'. Replace `make sdk` with `make anchor-build` (produces the .so and patches the IDL — what we actually need from the solana side) plus `bun run build` at the repo root, which builds every workspace package's TypeScript. Mirrors what cli/install.sh does for the EVM e2e job.
The transceiver_message PDA is scoped by config.key(), but nothing bound the message's recipient_ntt_manager to it, so a VAA addressed to instance A could be used to create a (redundant, and ultimately unredeemable) transceiver message under instance B. redeem already enforces the binding for fund safety; also check it in both transceivers' receive_message so the mis-scoped account is never created.
create-spl-multisig was always calling NTT.pdas(managerKey) without the instance pubkey, so for v4 multi-tenant deployments the multisig would be created with the wrong (v3-style) token authority as its member.
de8534b to
63910c4
Compare
The quoter IDL is only emitted under idl/4_0_0/ts/, not idl/3_0_0/ts/, so the 3_0_0 import path did not resolve and broke the SDK TS build (TS2307), failing every CI job that compiles sdk-solana-ntt.
The SHA de0fac2e... is tag v6.0.2, but cli.yml:154 commented it as v6 (which now floats to a different commit), tripping zizmor's ref-version-mismatch audit. Match the other 20 pins in the repo.
For a v4 (multi-tenant) manager the token_authority PDA is scoped by the instance, but set-mint-authority derived it with NTT.pdas(manager) (legacy/v3), so on a deployed v4 manager it would target the wrong PDA and set the mint authority to an address the program cannot sign for. Mirror the fix already applied to svm create-spl-multisig in solana.ts: use solanaNtt.pdas.tokenAuthority() on the deployed path (instance read from the deployment file) and add an --instance flag for the undeployed path via NTT.pdas(manager, instance).
Mirror the standalone transceiver's sanity check (which mdulin2 noted is missing from the ntt-transceiver copy): assert outbox_item.manager == config.key() so an outbox item from another instance can't be released here. The mark_outbox_item_as_released CPI already enforces this; this is a second layer.
Add the RegisteredTransceiver seed derivation mdulin2 noted is present in the standalone transceiver but missing here. Because the account is owned by the manager program, the derivation uses seeds::program = example_native_token_transfers::ID. Already validated by the CPI; this is a second-layer sanity check.
multi_instance.test.ts and upgrade_authority_decoupling.test.ts used the CommonJS __dirname global, which is undefined under jest's ESM mode, so both suites failed to load with ReferenceError. This was masked while the SDK build (Setup SDK step) was failing earlier in the job. Derive it from import.meta.url, mirroring anchor.test.ts.
Yes, no Fogo specific changes here. Added a paragraph in the README that explains these changes conform to the NTT spec in a backwards compatible way, so no SVM <-> SVM funny business (and more generally, a v4 NTT instance can be registered against older EVM/Sui/SVM/etc. NTTs) |
johnsaigle
left a comment
There was a problem hiding this comment.
Hey @kcsongor, I noticed some other bugs in the TS portion. Could you please review the below issues and fix them if you agree?
-
pushregisters the shared Solana program ID (manager), not the v4 instance. This can register peers that send transfers to an address a v4 instance cannot redeem.
cli/src/validation.ts:L278 -
push bypasses
canUpgrade. A config push can apply a v3-to-v4 upgrade path and strand custody.
cli/src/config-mgmt.ts:L83 -
canUpgradepermits v4-to-v3 downgrade.
cli/src/upgradeBarriers.ts:L55 -
upgradeSolanadoes not pass the existing instance. This can create a new random authority and block v4 burning-mode upgrades.
Evidence: cli/src/solana/deploy.ts:L732 -
Manual commands omit
chainConfig.instance. v4 peer setup and emergency redeem flows cannot build instance-scoped Solana state.
cli/src/commands/manual.ts:L111 -
addSolanaInstancesuppresses registration failures. add-chain can persist an instance that cannot transfer.
cli/src/solana/deploy.ts:L705
|
Two more:
|
No description provided.